Abort mark-synced on a repeated non-transient 4xx - #142
Conversation
Distinguish a request-shape 4xx (400/422) from transient/per-record failures in markRecordSynced and stop the mark-synced run when a whole batch is rejected the same way, instead of firing every remaining PATCH. A confirmation probe of the last record guards against stranding valid records behind a contiguous block of per-record 4xx. 429/404/401/403/5xx stay transient/per-record and never abort. Closes #137
grimicorn-agent
added
the
has-suggestions
Independent code review trailAn independent Opus reviewer (which did not write the code) reviewed the diff over multiple rounds; each round's findings were applied before the next. Summary: Round 1 — classification too broad. Original design aborted on any non-429 4xx. Flagged that a 404 (a record deleted server-side between fetch and PATCH) is per-record, not request-shape, and would wrongly abort. Fixed: narrowed the fatal set to an explicit Round 2 — single 4xx aborted the whole run. A lone per-record 422 would strand every later record forever (the pending set re-fetches in the same order and re-aborts). Fixed: abort only when a whole batch is unanimously rejected AND nothing has synced yet. Round 3 — one-record tail batch / empty batch. Round 4 — contiguous bad block > batch width; headline over-claimed. A rejected batch that lands after unrelated failures mis-attributed the cause. Fixed: neutral headline; corrected Rounds 5-7 — abort had no confirmation. Aborting on the first rejected batch still stranded a contiguous per-record-bad block. Fixed: added a confirmation probe; then made it probe the last record (far from a front-loaded bad block); then made it report plain failures when nothing is left to strand. Added Rounds 8-10 — probe robustness. Fixed: probe capped at once per invocation (no hammering a rate-limited record); a probe-synced record tracked via Final round — control flow confirmed sound. The reviewer traced the boundary cases (single-record tail batch, rejected final batch, probe timeout, probe-synced index surviving a later stop) and each lands correctly. Intentionally not actioned (recorded as a follow-up in the PR body): confirming the abort with a single last-record probe leaves a rare tail case — if that last record is itself the one per-record-bad record, the abort is a false positive. A two-record probe would remove it. Left as a scoped follow-up rather than expanding this PR.
|
Reconcile the mark-synced abort-on-4xx work with main's bulk PATCH refactor: the per-record probe machinery is replaced by aborting a bulk chunk on a request-shape 400/422 (MARK_ABORTED), since the bulk endpoint never 4xxs per-record.
Independent review flagged that aborting on the first (or any two) 400/422 chunks could strand trailing records if a chunk ever rejects for a per-record reason. Abort only when a SECOND chunk carries the SAME error message with nothing synced yet (envelope-level evidence), collapse the redundant abort/stoppedBy fields into one ChunkStop, and drop the 'not attempted' clause when the abort left nothing unattempted.
- Require the two request-shape rejections to be CONSECUTIVE (reset the tracker on any non-request-shape chunk) so an interleaved failure can't confirm a stale match. - Tag request-shape chunks MARK_FAILED and re-tag only the stopping chunk MARK_ABORTED, so a completed run never leaves a stray MARK_ABORTED. - Name the ChunkStop discriminants (STOP_*) and bundle the failure-report reason+unattempted count into one MarkStopReport object.
- The generic failure headline now notes how many pending records were never sent when a systemic stop ended the run early, instead of implying all N failed. - Hoist the shared MARK_FAILED outcome list in markSyncedChunk.
Independent code review trail (post-merge)After merging Round 1 — abort-on-first-4xx could strand records. Aborting the whole run on the first 400/422 chunk assumed the bulk endpoint never 4xxs per-record. Also flagged: stale Round 2 — two-strike still not evidence-based. Two different per-record rejections across two chunks could satisfy a bare two-strike rule without being an envelope fault. Also: an abort on the final chunk claimed "the rest were not attempted" when everything was attempted. Fixed: require the two request-shape rejections to carry the same error message (envelope-level evidence); drop the "not attempted" clause when nothing was left unattempted. Round 3 — consecutiveness + honest outcome tags. A non-request-shape chunk between two identical rejections shouldn't count toward the abort; and tagging records Round 4 — systemic early stop wording. A systemic abort (401/403/429/5xx) fell to the generic "Failed to mark N synced" headline, implying all N were attempted. Fixed: the generic branch now notes how many pending records were never sent when the run stopped early. Also hoisted a duplicated Intentionally not actioned:
|
…uest-shape abort Unify main's request-shape 4xx abort (#142) with this PR's permanent/transient classification: MarkAbortReason now carries 'timeout' | 'permanent' | 'transient' | 'request-shape' | null as the single run-level discriminant (replacing main's stoppedBy), so both features coexist. Permanent (401/403) stops the daemon; transient (429/5xx) and request-shape abort the run but keep it alive.
2 participants
What & why
markpost sync's mark-synced step PATCHes written records synced. Before this change a request-shape failure — a malformed payload (400) or a contract-validation rejection (422, e.g. markpost tightening the PATCH attributes it accepts) — failed a chunk and every remaining chunk fired and failed identically. This aborts the run once it has evidence the request itself is categorically wrong, instead of firing more doomed PATCHes.Closes #137
How it works
src/libs/api.ts).ApiRequestError.isFatalRequest+ theisFatalRequestErrorguard flag only 400/422 as request-shape failures. A transient 429, a per-record 404, an auth 401/403, and any 5xx are deliberately excluded — they don't mean the request shape is wrong, so they never trigger a request-shape abort (429/401/403/5xx still abort as systemic,main's existing behavior).MARK_ABORTED(src/libs/records.ts). A bulk chunk rejected with a request-shape 4xx is taggedMARK_FAILEDwhile the run continues; only the chunk that actually stops the run is re-taggedMARK_ABORTED, so a completed run never leaves a strayMARK_ABORTED. A non-JSON error body at any status still degrades safely toMARK_FAILED.markRecordsSynced). The run aborts only once a second consecutive chunk is rejected with the SAME error message and nothing has synced yet. The CLI builds every chunk's payload identically, so two independently-built chunks failing the same categorical way is strong evidence the envelope shape is wrong. A lone rejection, two rejections with different messages (which look like isolated per-record problems), a rejection separated from the last by a clean/other-failure chunk, or any rejection after a success (a success proves the shape valid) all keep the run going rather than strand syncable records behind an unconfirmed abort.src/index.ts). An abort gets its own headline; the "the rest were not attempted" clause is dropped when the abort landed on the final chunk (nothing left unattempted), and a systemic early stop notes how many records were never sent.Key decisions
MARK_FAILEDunless the run actually aborts, so an outcome named "aborted" only ever appears on a run that did abort.markSyncedStopReason/probeStopReasonscaffolding is obsolete and was removed rather than revertingmain's bulk refactor.Tests
tests/libs/api.test.ts,tests/libs/records.test.ts,tests/index.test.ts: status-code boundaries (400/422 → request-shape; 404/401/403/429/5xx don't); the two-chunk-confirmation state machine (two matching rejections abort; different messages / interleaved failure / post-success rejection do not); the honestMARK_FAILED→MARK_ABORTEDre-tagging; and the abort/timeout/systemic reporting paths.npm run lint,npm run typecheck, and 795 tests pass.Viewable
CLI behavior; exercised via
markpost sync(the mark-synced step when autoDelete is off). No UI/URL.Follow-up suggestions
Classify the systemic mark-synced stop in the report— a systemic abort (401/403/429/5xx) falls to the generic "Failed to mark N synced" headline with no classified cause;describeSystemicFailurealready produces the actionable text (e.g. "Authentication failed (HTTP 401)…") and could be surfaced there so a cron log says why the run stopped (suggested: P3, effort: S, evidence: src/index.ts markFailureHeadline / src/libs/records.ts markRecordsSynced systemic branch)